Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Removing Redis from internal lua function names and comments #1102

Merged
merged 8 commits into from
Oct 4, 2024
Merged

Removing Redis from internal lua function names and comments #1102

merged 8 commits into from
Oct 4, 2024

Conversation

parthpatel
Copy link
Member

@parthpatel parthpatel commented Oct 1, 2024 •
edited by zuiderkwast
Loading

I couldn't figure out how to rebase on top of recent changes, so creating this new PR. This has changes to handle comments on previous PR #288.

@parthpatel
Fixed a bug in lua debugger for 'server *' invocations and Removing R…
d0691de 
…edis from internal function names.

* Fixed a bug where 'server *' invocation from lua debugger was not working.
* Functions in lua c files lack of documentation. Adding some documentation in this commit.
* I removed Redis from function names in eval.c and script_lua.c files. I did not touch log messages in this commit. I also did not touch references to "RedisProtocol".

Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
@parthpatel
Fixes based on PR feedback.
f46f084 
* Fixed comments to be 120 line length.
* Aliased error handler function.
* Renamed REDIS_LRAND... variable to SERVER...
* I did not rename RedisProtocol as it refers to the RESP protocol.

Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
@zuiderkwast zuiderkwast changed the title Rebasing the old PR on top of Valkey repo. (https://github.com/valkey-io/valkey/pull/288) Removing Redis from internal lua function names and comments Oct 1, 2024
@parthpatel
Fixing a merge conflict
f6887da 
Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
@codecov Codecov
Copy link

codecov bot commented Oct 1, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.62%. Comparing base (613e4e0) to head (2ddd7e6).
Report is 9 commits behind head on unstable.

Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #1102      +/-   ##
============================================
- Coverage     70.62%   70.62%   -0.01%     
============================================
  Files           114      114              
  Lines         61695    61717      +22     
============================================
+ Hits          43571    43585      +14     
- Misses        18124    18132       +8
Files with missing lines Coverage Δ
src/eval.c 56.73% <100.00%> (+0.14%) ⬆️
src/function_lua.c 99.17% <100.00%> (ø)
src/script_lua.c 90.16% <100.00%> (+0.01%) ⬆️
src/server.c 88.66% <ø> (ø)

... and 17 files with indirect coverage changes

@parthpatel
Formatting the code as per suggestions from the build process.
1c7b407 
Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
@parthpatel
Copy link
Member Author

Fixed all problems with the build. Next build will succeed. This is ready for review.

hpatro
hpatro reviewed Oct 2, 2024
View reviewed changes
Copy link
Contributor

@hpatro hpatro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, there were few tests in your previous PR to verify the availability of both the APIs.

src/script_lua.c Outdated Show resolved Hide resolved
@parthpatel
Fixing "customer message" typo
c1c1695 
Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
@parthpatel
Copy link
Member Author

IIRC, there were few tests in your previous PR to verify the availability of both the APIs.

@hpatro yeah. scripting.tcl has this block which will replace redis with server and test the same scripts.

foreach script_compatibility_api {server redis} {

# We run the tests using both the server APIs, e.g. server.call(), and valkey APIs, e.g. redis.call(),
# in order to ensure compatibility.
if {$script_compatibility_api eq "server"} {
    proc replace_script_redis_api_with_server {args} {
        set new_string [regsub -all {redis\.} [lindex $args 0] {server.}]
        return lreplace $args 0 0 $new_string
    }

    proc get_script_api_name {} {
        return "server"
    }
} else {
    proc replace_script_redis_api_with_server {args} {
        return {*}$args
    }

    proc get_script_api_name {} {
        return "redis"
    }
}

....

@parthpatel parthpatel requested a review from hpatro October 2, 2024 23:11
hwware
hwware reviewed Oct 2, 2024
View reviewed changes
src/eval.c Outdated Show resolved Hide resolved
@parthpatel
Resolving the conflict to keep Resp in the function names.
3a9f3c7 
Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
hpatro
hpatro approved these changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

@hpatro hpatro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid force-push on Github (if possible), one needs to go through the entire diff again😄.

@parthpatel
Copy link
Member Author

Avoid force-push on Github (if possible), one needs to go through the entire diff again😄.

noted :) I keep forgetting to add signed-off-by...
Although I did not squash the commit, last revision is in the last commit.

@parthpatel parthpatel requested a review from hwware October 3, 2024 22:38
zuiderkwast
zuiderkwast approved these changes Oct 4, 2024
View reviewed changes
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/script_lua.c Outdated Show resolved Hide resolved
parthpatel and others added 2 commits October 4, 2024 11:05
@parthpatel @madolson
Update src/script_lua.c to add __redis__err__handler as backup in the…
a5a9fe8 
… server_api_allo_list

Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: Parth <661497+parthpatel@users.noreply.github.com>
@parthpatel
Fixing a formatting issue due to extra space before a comment.
2ddd7e6 
Signed-off-by: Parth <661497+parthpatel@users.noreply.github.com>
@parthpatel parthpatel removed the request for review from hwware October 4, 2024 19:57
@parthpatel parthpatel removed the request for review from zuiderkwast October 4, 2024 19:57
@madolson madolson merged commit d8cd352 into valkey-io:unstable Oct 4, 2024
47 checks passed
naglera pushed a commit to naglera/placeholderkv that referenced this pull request Oct 10, 2024
@parthpatel @naglera
Removing Redis from internal lua function names and comments (valkey-…
f7a280d 
…io#1102)

Improved documentation and readability of lua code as well as removed references to Redis.

---------

Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
Signed-off-by: naglera <anagler123@gmail.com>
SoftlyRaining pushed a commit to SoftlyRaining/valkey that referenced this pull request Oct 11, 2024
@parthpatel @SoftlyRaining
Removing Redis from internal lua function names and comments (valkey-…
984f6f7 
…io#1102)

Improved documentation and readability of lua code as well as removed references to Redis.

---------

Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
eifrah-aws pushed a commit to eifrah-aws/valkey that referenced this pull request Oct 20, 2024
@parthpatel @eifrah-aws
Removing Redis from internal lua function names and comments (valkey-…
7eeeb45 
…io#1102)

Improved documentation and readability of lua code as well as removed references to Redis.

---------

Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hwware hwware hwware left review comments

@zuiderkwast zuiderkwast zuiderkwast approved these changes

@madolson madolson madolson approved these changes

@hpatro hpatro hpatro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@parthpatel @zuiderkwast @hpatro @madolson @hwware